Sign in with QR (MSC3906) compatibility with Rust Crypto #3761
Conversation
hughns
changed the title
|
@BillCarsonFr @richvdh If helpful I can progress this PR to get the simple changes that work when moving from |
|
I'm not sure I have full context on this (maybe @BillCarsonFr knows more?) and I'm not quite clear what you're proposing, but any contributions that increase compatibility with Rust Crypto sound helpful! |
|
Opened an issue to track this work: element-hq/element-web#26350 |
Thanks @hughns, this work is tracked on our side and identified as required for initial release of web-r, but not a blocker to release on develop.io. We will get to it after releasing to webR. As per your changes doesn't look like any API is missing? contrary to mobile side? |
hughns
force-pushed
the
hughns/msc3906-rust-crypto
branch
from
75de571 to
4c626f2
Compare
hughns
force-pushed
the
hughns/msc3906-rust-crypto
branch
from
4c626f2 to
9480b7b
Compare
hughns
force-pushed
the
hughns/msc3906-rust-crypto
branch
from
c37e313 to
2b832db
Compare
hughns
force-pushed
the
hughns/msc3906-rust-crypto
branch
from
106c9e8 to
a44abab
Compare
@BillCarsonFr I've done some more work on the PR. I have found an API missing (or I can't find it, at least) on the Web R side which is the ability to programatically cross-sign another device. The place in the code that this is done is Is it possible to get this exposed or for the behaviour of setDeviceVerified to be backwards compatible? |
hughns
force-pushed
the
hughns/msc3906-rust-crypto
branch
from
a44abab to
9f7f743
Compare
Unfortunately this link is 404ing :( |
I guess it means here |
I think it's probably best we distinguish clearly between the cross-signing case and the "mark as locally verified" case, not least because we can mark other users' devices as locally verified, but we cannot cross-sign them. I can add a new method to |
This is done in #3930 |
hughns
force-pushed
the
hughns/msc3906-rust-crypto
branch
from
9f7f743 to
fef5318
Compare
src/rendezvous/MSC3906Rendezvous.ts
Outdated
| master_key: masterPublicKey, | ||
| }); | ||
|
|
||
| return info; | ||
| } | ||
|
|
||
| /** | ||
| * Verify the device and cross-sign it. | ||
| * @param timeout - time in milliseconds to wait for device to come online | ||
| * @returns the new device info if the device was verified |
There was a problem hiding this comment.
this documentation looks to be outdated. Please could you correct it.
There was a problem hiding this comment.
(changing the return type is technically a breaking change. @t3chguy are you happy for this to land without a major version bump?)
There was a problem hiding this comment.
Taking silence as acceptance here. Nothing should be using this method outside the js-sdk anyway.
| type UserID = string; | ||
| type DeviceID = string; | ||
| type Fingerprint = string; |
There was a problem hiding this comment.
I'm not entirely convinced this is useful. As I understand it, this doesn't actually do much at the typescript level (ie, it won't complain if you end up using a DeviceID where it wanted a UserID), so the only purpose of such type aliasing is documentation... and frankly, I'd prefer to see documentation done as comments rather than as types.
I'm not going to insist on changing anything here: rather sharing a thought for future reference.
There was a problem hiding this comment.
Sorry, there's a mix of comments here. Some of it is fairly unimportant stuff that I failed to notice on the previous review, so maybe not worth fixing now.
But it feels like you changed the return type of mockDevice and then failed to follow that change through, by simplifying the functions that call it. I'd like to see that finished.
Currently the implementation of Sign in with QR (MSC3906) uses the deprecated crypto functions and relies on some capabilities not yet provided by the Rust crypto implementation. Required for element-hq/element-web#26350
CC @BillCarsonFr @richvdh
Checklist
This change is marked as an internal change (Task), so will not be included in the changelog.